MailboxesParser by piernik · Pull Request #268 · ddeboer/imap

piernik · 2017-11-20T14:58:34Z

I'm adding mailboxesParser. It sorts mailboxes and fetches special folders like "sent", "inbox", "trash". Tests are in tests\MailboxesParser\.
Usage:

$parser = new MailboxesParser($this->mailboxes);
//set language (only pl) for special folders names and mailboxNames
$parser->setLanguage('pl');
//get flat but sorted folders. Folders are ParsedMailbox class
$folders = $parser->getFolders();
//get tree structure of folders
$tree = $parser->getTreeStructure();

This reverts commit 8919812

Slamdunk

Hi, the idea is good, we need to shape it for general purpose though.

Please make the tests pass, and run the PHP-CS-Fixer to apply the code style this library follows.

Slamdunk · 2017-11-27T10:39:01Z

.gitignore

 composer.lock
 phpunit.xml
+.idea/
+phpunit.xml.dist


We need phpunit.xml.dist in the package, please revert .gitignore file

Slamdunk · 2017-11-27T10:39:21Z

phpunit.xml.dist

-    verbose="true"
+        bootstrap="./vendor/autoload.php"
+        colors="true"
+        verbose="true"


We want to keep one indentation for this file, please revert it.

Slamdunk · 2017-11-27T10:43:53Z

src/MailboxesParser/MailboxesParser.php

+        self::TRASH     => ['trash', 'bin', 'INBOX.trash', '[Gmail]/trash'],
+        self::TEMPLATES => ['templates'],
+        self::ARCHIVES  => ['archives',],
+    ];


ID of special folder vary a lot from different email provider, I don't want to have a static list to keep up to date.

What about letting the user specify it in the constructor, so this library doesn't have to maintain it?

I think those are common ones. All other folders are simply listed

Unfortunately not, those are not as common as you may think.
I work with a lot of IMAP servers where these special folders have many other aliases: only for the Trash one I have 15 variants 😞 in English, without counting localizations.

I'm still against to maintain this list: let the user specify them.

Also the tests would be much more easy to implement and verify.

Slamdunk · 2017-11-27T10:44:37Z

src/MailboxesParser/MailboxesParser.php

+        self::TRASH     => 'Trash',
+        self::TEMPLATES => 'Templates',
+        self::ARCHIVES  => 'Archives',
+    ];


The same goes for folder names: if you need them, let the user specify them as a constructor dependency.

We don't have the resources to maintain translations.

You can manually name special folders with setSpecialFoldersNames.
and add special folders Id with addSpecialFolderId

Slamdunk · 2017-11-27T10:44:58Z

src/MailboxesParser/MailboxesParser.php

+     *
+     * @param MailboxInterface[] $mailboxes
+     */
+    public function __construct($mailboxes)


Please add type-hints

I think in php 7.0 I cannot use public function __construct(MailboxInterface[] $mailboxes)

No, but you can typehint the array at least.

I still see __construct($mailboxes).

Can you change this to __construct(array $mailboxes)?

Slamdunk · 2017-11-27T10:48:10Z

src/MailboxesParser/MailboxesParser.php

+        $this->specialFoldersIds[$specialFolder][] = $id;
+    }
+
+    private function getFolderLevel($mailboxName, $delimiter)


Please add type-hints, return-type and doc-block

Slamdunk · 2017-11-27T10:48:28Z

src/MailboxesParser/MailboxesTreeParser.php

+namespace Ddeboer\Imap\MailboxesParser;
+
+
+class MailboxesTreeParser


Please make it final and add an Interface for it

Slamdunk · 2017-11-27T10:48:44Z

src/MailboxesParser/MailboxesTreeParser.php

+     *
+     * @return array
+     */
+    public function parse($input)


Please add type-hints and the return-type

Slamdunk · 2017-11-27T10:49:03Z

src/MailboxesParser/ParsedMailbox.php

+
+use Ddeboer\Imap\MailboxInterface;
+
+class ParsedMailbox


Please make it final and add an Interface for it

Slamdunk · 2017-11-27T10:49:47Z

src/MailboxesParser/ParsedMailbox.php

+    /**
+     * @param MailboxInterface $mailbox
+     */
+    public function setMailbox(MailboxInterface $mailbox)


Can we move all the setters to be the constructor?

this is internal helper class I see no point.

The point is that immutable objects are far more easy to maintain.

piernik · 2017-12-01T09:33:26Z

I've changed files.

Slamdunk · 2017-12-01T09:39:56Z

Thanks for the changes.

Still please make the tests pass, and run the PHP-CS-Fixer to apply the code style this library follows.

piernik · 2017-12-01T10:06:01Z

I said I'm not github fluent. I don't know what that is :)
Can't You make it on Your own?

Slamdunk · 2017-12-01T10:13:16Z

On the bottom of this Pull Request you can see a red cross near continuous-integration/travis-ci/pr.

If you click on the "Details" link you can see that Travis, a continuous integration server, pulled your PR and performed some checks that this library asked Travis to do.

At the time of writing, those checks are done against three different PHP versions, and if you click in each of them you can see what are the failures that are marking your PR as failing.

The first failure I see is:

PHP Fatal error:  Class 'PHPUnit_Framework_TestCase' not found in /home/travis/build/ddeboer/imap/tests/MailboxesParser/MailboxesParserTest.php on line 15

And the second failure I see is that you didn't run the following command:

$ vendor/bin/php-cs-fixer fix --verbose

That fix the coding style of the code you wrote.

Please try to fix those failure: if you need more help I'm here.

piernik · 2017-12-01T10:30:50Z

About tests - You're using phpunit 6 - I have phpunit 5 globally. It's fixed.

About cs-fixer. This is my output. Don't know what that means.

Slamdunk · 2017-12-01T10:34:46Z

Vendor binaries must be run from the project root folder.
All the necessary dependencies are installed in the project: you should run these once, there is no need for a global PHPUnit installation.

From the imap project repository run these commands:

$ vendor/bin/phpunit
$ vendor/bin/php-cs-fixer fix --verbose

You need both to pass.

piernik · 2017-12-01T10:57:44Z

I give up. Fix the code on Your own - sorry.

Slamdunk · 2017-12-01T11:07:45Z

I'm sad that you are giving up.

Though, each new functionality must be well tested and checked.
This library is widely used by a lot of users and we must ensure reliability: we can ship code that isn't tested.

I'll leave this PR open: if you ever want to finish it, we'll be thankful.

piernik · 2017-12-01T11:25:31Z

I did add tests. I don't know what travis wants. You can fetch code and fix it on Your own.

Slamdunk · 2017-12-04T09:33:08Z

I'm sorry but you can't share a code that isn't proved to be working and expect someone else to fix it.

I can help you if you don't understand the tools, but it is up to you to provide the functionality and its tests, and to prove everything works fine.

piernik and others added 7 commits November 8, 2017 13:54

uid in appended message as return

8919812

Mailboxes parser

9da10aa

Revert "uid in appended message as return"

62827ce

This reverts commit 8919812

Merge branch 'mailboxesparser'

e1b9bd5

mailboxes parser fixes

c47ca92

Update phpunit.xml.dist

65c9841

Update phpunit.xml.dist

3868527

This was referenced Nov 21, 2017

Parser for eml files? #265

Open

Suggestion: tree structure of mailboxes #261

Closed

Slamdunk requested changes Nov 27, 2017

View reviewed changes

Slamdunk added the enhancement label Nov 27, 2017

piernik added 2 commits December 1, 2017 10:31

mailboxes parser docs and interfaces

ebb9271

Merge remote-tracking branch 'origin/master'

d0f855f

tests fixes

bbd78f1

piernik added 2 commits December 1, 2017 11:42

tests fixes

7d5bed6

tests fixes

bb972bc

Slamdunk added the awaiting author updates label Dec 7, 2017

		namespace Ddeboer\Imap\MailboxesParser;


		class MailboxesTreeParser

Uh oh!

Conversation

piernik commented Nov 20, 2017

Uh oh!

Slamdunk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slamdunk Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piernik commented Dec 1, 2017

Uh oh!

Slamdunk commented Dec 1, 2017

Uh oh!

piernik commented Dec 1, 2017

Uh oh!

Slamdunk commented Dec 1, 2017

Uh oh!

piernik commented Dec 1, 2017

Uh oh!

Slamdunk commented Dec 1, 2017

Uh oh!

piernik commented Dec 1, 2017

Uh oh!

Slamdunk commented Dec 1, 2017

Uh oh!

piernik commented Dec 1, 2017

Uh oh!

Slamdunk commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Slamdunk Dec 1, 2017 •

edited

Loading

Slamdunk commented Dec 4, 2017 •

edited

Loading